Set base address on OSX #313

Timmmm · 2018-02-06T13:12:37Z

This fixes issue #311. The mapped address of libraries was never considered, nor was the load address of the segments.

This fixes issue google#311. The mapped address of libraries was never considered, nor was the load address of the segments.

codecov-io · 2018-02-06T13:19:29Z

Codecov Report

Merging #313 into master will decrease coverage by 0.05%.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##           master    #313      +/-   ##
=========================================
- Coverage   65.86%   65.8%   -0.06%     
=========================================
  Files          36      36              
  Lines        7402    7429      +27     
=========================================
+ Hits         4875    4889      +14     
- Misses       2132    2142      +10     
- Partials      395     398       +3

Impacted Files	Coverage Δ
internal/binutils/binutils.go	`56.75% <0%> (-2.82%)`	⬇️
internal/binutils/disasm.go	`89.04% <77.77%> (-2.14%)`	⬇️
internal/driver/webui.go	`59.4% <0%> (+0.95%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8f279b...25f4ba0. Read the comment docs.

aalexand · 2018-02-06T16:19:32Z

I can accept the fix as is because it's an improvement we need, but how about a test?

Timmmm · 2018-02-06T17:12:46Z

Sure. What should it test exactly though? I could throw a small mach-o library and binary in there to check getTextAddress() works?

aalexand · 2018-02-06T18:08:02Z

Yes, a test similar to the ones in https://github.com/google/pprof/blob/master/internal/binutils/binutils_test.go against a canned binary from testdata. The test will need to be skipped on anything non-Mac.

There is already a function very similar to getTextAddress(). This also adds a test that tries reading symbols / source addresses from a library and an executable. I also had to fix a couple of bugs for the tests to work: 1. Crash when the regex passed to f.Symbols() was nil. 2. The last symbol returned by `nm` is always discarded.

Timmmm · 2018-02-07T17:20:39Z

I added a test. The test actually found some bugs elsewhere in the code. Also I'm not 100% sure about the intent of some of the test I copied it from. Anyway the test passes and I guess it was useful as it contains bug fixes.

aalexand

Do files internal/binutils/debug.test, internal/binutils/testdata/exe_mac_64.dSYM/Contents/Info.plist, internal/binutils/testdata/exe_mac_64.dSYM/Contents/Resources/DWARF/exe_mac_64, internal/binutils/testdata/lib_mac_64.dSYM/Contents/Info.plist, internal/binutils/testdata/lib_mac_64.dSYM/Contents/Resources/DWARF/lib_mac_64 need to be part of the PR?

aalexand · 2018-02-07T17:26:47Z

internal/binutils/binutils.go

@@ -179,10 +179,20 @@ func (b *binrep) openMachO(name string, start, limit, offset uint64) (plugin.Obj
 	}
 	defer of.Close()

+	base := start


I am a bit hesitant whether a better value for the base in the absence of __TEXT is zero or start. But I guess it's not important since files without __TEXT should be impractical case.

Yeah I'll add a warning if it isn't found.

aalexand · 2018-02-07T17:29:18Z

internal/binutils/binutils_test.go

+				t.Fatalf("Symbols: unexpected error %v", err)
+			}
+
+			find := func(name string) *plugin.Sym {


Nit: could move this piece out to an outer function to reuse with the other test.

aalexand · 2018-02-07T17:56:32Z

internal/binutils/disasm.go

@@ -62,7 +69,7 @@ func matchSymbol(names []string, start, end uint64, r *regexp.Regexp, address ui
 		return names
 	}
 	for _, name := range names {
-		if r.MatchString(name) {
+		if r == nil || r.MatchString(name) {


Do we pass r as nil from anywhere? Ah, you do know in f.Symbols(nil, 0) in the test. I think you can just do f.Symbols(regexp.MustCompile(tc.sym), 0)?.

No but the comments for the Symbols() interface say it can be nil.

aalexand · 2018-02-07T18:00:28Z

internal/binutils/disasm.go

@@ -37,6 +37,7 @@ func findSymbols(syms []byte, file string, r *regexp.Regexp, address uint64) ([]
 	var symbols []*plugin.Sym
 	names, start := []string{}, uint64(0)
 	buf := bytes.NewBuffer(syms)
+	symAddr := uint64(0)


I think this symAddr will get shadowed by the assignment in the loop header, so it's probably not what the code after the loop expects.

Well spotted!

aalexand · 2018-02-07T18:03:12Z

@Timmmm Huh, Mac tests fail on Travis with

--- FAIL: TestMachoFiles (0.62s)
    --- FAIL: TestMachoFiles/normal_mapping (0.57s)
    	binutils_test.go:325: SourceLine for main: got [{_main  0}]; want [{main /tmp/hello.c 3}]
    --- FAIL: TestMachoFiles/other_mapping (0.02s)
    	binutils_test.go:325: SourceLine for main: got [{_main  0}]; want [{main /tmp/hello.c 3}]
    --- FAIL: TestMachoFiles/lib_normal_mapping (0.03s)
    	binutils_test.go:325: SourceLine for main: got [{_bar  0}]; want [{bar /tmp/lib.c 6}]

aalexand · 2018-02-07T20:51:23Z

Hoisting out this question from the last round of comments because those top-level Github comments are easy to miss so not sure you saw it:

Do files internal/binutils/debug.test, internal/binutils/testdata/exe_mac_64.dSYM/Contents/Info.plist, internal/binutils/testdata/exe_mac_64.dSYM/Contents/Resources/DWARF/exe_mac_64, internal/binutils/testdata/lib_mac_64.dSYM/Contents/Info.plist, internal/binutils/testdata/lib_mac_64.dSYM/Contents/Resources/DWARF/lib_mac_64 need to be part of the PR?

Timmmm · 2018-02-07T21:08:45Z

Ah yeah I missed that, and yes they do, except debug.test. Not sure where that came from. I can't work out why the travis test fails. I just downloaded it on a totally separate mac and the test passes for me. I wonder if Travis uses addr2line instead of llvm-symbolizer and there is some difference there?

Timmmm · 2018-02-07T21:15:53Z

Yeah that seems to be it. Removing llvm-symbolizer and installing binutils (for gaddr2line) gives the exact same failure as above.

However gaddr2line does seem to work when I run it manually, so I guess there is another bug in pprof around that. I'll look into it tomorrow. I guess you guys don't use Macs!

aalexand · 2018-02-07T21:26:33Z

@Timmmm A lot of development here happens on Linux. I carry a Mac laptop but a lot of times I'm just in an SSH window to my Linux machine. And when I do use it for actual coding, that's Go and I'm not hitting the issues you've found because the Golang profiles are pre-symbolized by the runtime.

Thanks for figuring many things out here!

* Return an error if __TEXT isn't found * Return an error if start - textSegment.addr would underflow * Break out findSymbol() function * Rework findSymbols() function, it now returns non-EOF errors instead of ignoring them * Remove accidentally added debug binary

Timmmm · 2018-02-08T10:43:35Z

I've updated it as requested. The test will still fail because pprof because of a number of issues:

pprof only looks for addr2line but on my system at least it is called gaddr2line. No idea why. Probably a stupid reason. It should search for both anyway. I wasn't sure how you would want to handle that so I'll leave it to you.
When using addr2line there is some code that attempts to "improve" the symbol names using nm. Unfortunately the names nm returns are mangled (it is executed as nm -n to prevent demangling), so it finds _main instead of main and the test fails.
If there is no addr2line or llvm-symbolizer the test will clearly fail, but I guess that case is ok because the test environment should have one of those.

I'd suggest the easiest thing for now is to install llvm-symbolizer on the Mac test environment so the test passes, and open issues for the other things. I can't really spend any more time on this sorry!

Timmmm · 2018-02-08T10:46:41Z

Oh yeah I also forgot to ask - is the logic behind matchSymbol() actually correct? If the symbol is matched by address, all of the names are returned, but for regex matching only the first one is. Seems suspicious to me.

aalexand · 2018-02-09T03:19:40Z

internal/binutils/binutils.go

+
+	textSegment := of.Segment("__TEXT")
+	if textSegment == nil {
+		return nil, fmt.Errorf("Parsing %s: no __TEXT segment", name)


Error messages should start with lowercase: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Suggesting fmt.Errorf("not a mach executable: no __TEXT segment: %s", name), to be somewhat consistent with fmt.Errorf("unrecognized binary: %s", name) above in this file.

I just copied the one above but ok.

aalexand · 2018-02-09T03:25:12Z

internal/binutils/binutils.go

+		return nil, fmt.Errorf("Parsing %s: no __TEXT segment", name)
+	}
+	if textSegment.Addr > start {
+		return nil, fmt.Errorf("Parsing %s: __TEXT segment address (0x%x) > mapping start address (0x%x)",


Error messages should start with lowercase: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Suggesting fmt.Errorf("not a mach executable: __TEXT segment address (0x%x) > mapping start address (0x%x): %s", testSegment.Addr, start, name).

By the way, could base be negative in case of some exotic executable ASLR? Probably not, I think in reality for PIE executables textSegment address is zero, at least that's what I saw on Linux.

Yeah I wasn't sure about that but even if it is you'd probably want to know about it - i.e. get a bug report here, rather than just having it underflow and fail silently (or get lucky with the unsigned arithmetic).

aalexand · 2018-02-09T03:27:03Z

internal/binutils/disasm.go

+	for {
+		symAddr, name, err := nextSymbol(buf)
+		if err == io.EOF {
+			// Done! If there was an unfinished group, append it.


Nit / opiniated: remove the exclamation mark, those are overly emotional for comments and log messages to my taste...

aalexand · 2018-02-09T03:35:02Z

internal/binutils/testdata/lib_mac_64.dSYM/Contents/Info.plist

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>


Reading https://stackoverflow.com/questions/8058497/can-clang-build-executables-with-dsym-debug-information, it appears to me that building the test binaries from the command line it should be possible to have the debug information as part of those, and these additional dSYM files and directory would not be needed. I can look into doing that myself, can you include the source code of the executable and library test binaries that you used?

Yep you can build it from source too. I just copied how the Linux test worked.

The only weird thing is if you have linking as a separate step (i.e. clang++ -c foo.cpp -o foo.o; clang++ foo.o -o foo rather than just doing it all in one step you need to run dysmutil on the binary. Shouldn't matter in this case because it is just one file.

aalexand · 2018-02-09T03:38:18Z

@timm I left some comments, those are minor, please address.

I apologize it took so much longer than I thought it would. I'd probably suggest that you leave the test code in as is, but make the mac test skipped permanently, and I'll look into enabling that.

For matchSymbol behavior - that code pre-dates me on the project, I found the behavior a bit unexpected as well, it should be at least documented better, will look at it later.

Timmmm · 2018-02-09T10:05:26Z

Ok I fixed some of the nits (getting a little extreme!) and lowercased other error messages. I didn't use your exact error message suggestions because they still are mach files - and there was already an error message for failing to find the base address for ELF so I copied that.

The source I used was:

/tmp/hello.c

#include <stdio.h>

int main() {
	printf("Hello, world!\n");
	return 0;
}

/tmp/lib.c

int foo() {

	return 1;	
}

int bar() {

	return 2;
}

Compiled with clang -g /tmp/hello.c -o exe_mac_64 and clang -g /tmp/lib.c -dynamiclib -o lib_mac_64 (which should generate the dSYM's in both cases).

aalexand · 2018-02-09T16:49:19Z

@Timmmm LGTM, merging. Thanks for bearing with those nit comments. And thanks for this fix overall. I'll get the test fixed.

Set base address on OSX. This fixes issue google#311. The mapped address of libraries was never considered, nor was the load address of the segments. It also fixes a couple of issues in binutils.go, such as when the tail of the data wasn't handled right when grouping symbols at the same address.

Set base address on OSX

5bdb9a9

This fixes issue google#311. The mapped address of libraries was never considered, nor was the load address of the segments.

googlebot added the cla: yes label Feb 6, 2018

aalexand reviewed Feb 7, 2018

View reviewed changes

aalexand reviewed Feb 9, 2018

View reviewed changes

Fix nits & remove unused test binaries for now

25f4ba0

aalexand approved these changes Feb 9, 2018

View reviewed changes

aalexand merged commit 3a72dae into google:master Feb 9, 2018

This was referenced Feb 14, 2018

Enable MacOS binutils test. #322

Merged

Not able to see the profiling results properly for a profile generated from gperftools on mac OS High sierra #324

Open

cosnicolaou mentioned this pull request May 24, 2023

cmd/pprof: disasm not working on darwin golang/go#50891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set base address on OSX #313

Set base address on OSX #313

Timmmm commented Feb 6, 2018

codecov-io commented Feb 6, 2018 •

edited

Loading

aalexand commented Feb 6, 2018

Timmmm commented Feb 6, 2018

aalexand commented Feb 6, 2018

Timmmm commented Feb 7, 2018

aalexand left a comment

aalexand Feb 7, 2018

Timmmm Feb 7, 2018

aalexand Feb 7, 2018

Timmmm Feb 7, 2018

aalexand Feb 7, 2018

Timmmm Feb 7, 2018

aalexand Feb 7, 2018

Timmmm Feb 7, 2018

aalexand commented Feb 7, 2018

aalexand commented Feb 7, 2018

Timmmm commented Feb 7, 2018

Timmmm commented Feb 7, 2018

aalexand commented Feb 7, 2018

Timmmm commented Feb 8, 2018

Timmmm commented Feb 8, 2018

aalexand Feb 9, 2018

Timmmm Feb 9, 2018

aalexand Feb 9, 2018

Timmmm Feb 9, 2018

aalexand Feb 9, 2018

aalexand Feb 9, 2018

Timmmm Feb 9, 2018

aalexand commented Feb 9, 2018

Timmmm commented Feb 9, 2018

aalexand commented Feb 9, 2018

Set base address on OSX #313

Set base address on OSX #313

Conversation

Timmmm commented Feb 6, 2018

codecov-io commented Feb 6, 2018 • edited Loading

Codecov Report

aalexand commented Feb 6, 2018

Timmmm commented Feb 6, 2018

aalexand commented Feb 6, 2018

Timmmm commented Feb 7, 2018

aalexand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Feb 7, 2018

aalexand commented Feb 7, 2018

Timmmm commented Feb 7, 2018

Timmmm commented Feb 7, 2018

aalexand commented Feb 7, 2018

Timmmm commented Feb 8, 2018

Timmmm commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Feb 9, 2018

Timmmm commented Feb 9, 2018

aalexand commented Feb 9, 2018

codecov-io commented Feb 6, 2018 •

edited

Loading